Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce SqliteObserver and use it to collect stats related to queries and db size #2707

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

shrima-cf
Copy link
Contributor

@shrima-cf shrima-cf commented Sep 12, 2024

@shrima-cf shrima-cf marked this pull request as ready for review September 12, 2024 23:11
@shrima-cf shrima-cf requested review from a team as code owners September 12, 2024 23:11
src/workerd/util/sqlite.h Outdated Show resolved Hide resolved
src/workerd/util/sqlite.h Outdated Show resolved Hide resolved
src/workerd/util/sqlite.h Show resolved Hide resolved
src/workerd/util/sqlite.c++ Outdated Show resolved Hide resolved
src/workerd/util/sqlite.c++ Outdated Show resolved Hide resolved
src/workerd/util/sqlite-test.c++ Show resolved Hide resolved
@shrima-cf shrima-cf force-pushed the shrima/STOR-3710-add-SqliteObserver branch from 153a9a2 to aab4f76 Compare September 13, 2024 17:08
@shrima-cf shrima-cf force-pushed the shrima/STOR-3710-add-SqliteObserver branch from aab4f76 to ee5e13c Compare September 13, 2024 20:08
@a-robinson a-robinson removed their request for review September 13, 2024 20:23
src/workerd/util/sqlite.c++ Outdated Show resolved Hide resolved
src/workerd/util/sqlite.c++ Outdated Show resolved Hide resolved
src/workerd/util/sqlite.h Outdated Show resolved Hide resolved
src/workerd/util/sqlite.h Outdated Show resolved Hide resolved
@shrima-cf shrima-cf force-pushed the shrima/STOR-3710-add-SqliteObserver branch 3 times, most recently from ce7963b to 5dee08b Compare September 13, 2024 21:17
@shrima-cf shrima-cf force-pushed the shrima/STOR-3710-add-SqliteObserver branch from 5dee08b to 5e2e412 Compare September 16, 2024 16:20
@shrima-cf
Copy link
Contributor Author

@justin-mp The new changes are the ones from here

@shrima-cf shrima-cf force-pushed the shrima/STOR-3710-add-SqliteObserver branch from 5e2e412 to 0ea3375 Compare September 16, 2024 17:07
Copy link
Contributor

@justin-mp justin-mp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The makeActorCache changes seem reasonable to me.

src/workerd/util/sqlite.h Show resolved Hide resolved
src/workerd/util/sqlite.h Outdated Show resolved Hide resolved
@@ -1096,6 +1105,8 @@ void SqliteDatabase::Query::nextRow() {
db.currentRegulator = regulator;

int err = sqlite3_step(statement);
rowsRead = getRowsRead();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems inefficient as we're querying SQLite for rows read/written counts on every single row just in case the database is later resent before the query is destroyed -- which is very rare.

Instead, how about putting code into Query::beforeSqliteReset() that posts the metrics early (calls addQueryStats directly), and then have the destructor only post metrics if it hasn't been reset (i.e. maybeStatement is non-null).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try that approach first. The problem with that comes in the way we do getRowsRead() and the way the Statement and Query are setup. There are a couple of corner cases that have to be handled - when the Query has its own statement vs passed in a statement, handling the beforeSqliteReset in both Query and Statement.
In the end, I decided to go this route as it is easy to reason about.

As far as inefficiency in querying Sqlite for rows read/written, looking at the implementation of getRowsRead/Written, it does a sqlite3_stmt_status(...) which does a pointer read, so its not as inefficient as we think it is

Copy link
Contributor Author

@shrima-cf shrima-cf Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a todo with more details

src/workerd/util/sqlite.h Show resolved Hide resolved
@shrima-cf shrima-cf force-pushed the shrima/STOR-3710-add-SqliteObserver branch from 0ea3375 to 4b48d70 Compare September 16, 2024 19:34
@shrima-cf shrima-cf force-pushed the shrima/STOR-3710-add-SqliteObserver branch from 4b48d70 to 222d244 Compare September 16, 2024 20:10
@shrima-cf shrima-cf merged commit 4d9d2f1 into main Sep 16, 2024
12 of 13 checks passed
@shrima-cf shrima-cf deleted the shrima/STOR-3710-add-SqliteObserver branch September 16, 2024 20:36
shrima-cf added a commit that referenced this pull request Sep 16, 2024
…bserver

Introduce SqliteObserver and use it to collect stats related to queries and db size
shrima-cf added a commit that referenced this pull request Sep 16, 2024
…bserver

Introduce SqliteObserver and use it to collect stats related to queries and db size
shrima-cf added a commit that referenced this pull request Sep 16, 2024
…bserver

Introduce SqliteObserver and use it to collect stats related to queries and db size
shrima-cf added a commit that referenced this pull request Sep 16, 2024
…bserver

Introduce SqliteObserver and use it to collect stats related to queries and db size
shrima-cf added a commit that referenced this pull request Sep 16, 2024
…bserver

Introduce SqliteObserver and use it to collect stats related to queries and db size
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants